Removed manual conversion to local time in date conversions #55
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ablu
changed the title
|
Interesting... The Appveyor build fails when comparing |
DateTime overloads the - operator.
|
@spahnke would be cool if you could check whether the tests make sense. Date stuff is always ugly... :/. @oliverbock should work now :) |
|
somehow github did not handle my forced push to my branch... |
JavaScript usually does not care a lot about timezones, but since the conversion happens via milliseconds since epoch (which is UTC based) one needs to be a bit careful about which conversions to do where. Added/moved tests to a central place so there is an overview over all date related tests.
|
Ok. A second forced push fixed it |
|
I will review this on Monday. |
|
CI failure is an unrelated test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Calling
ToLocalTime()leads to changes of the hour component. Since in JavaScript Date is basically simply a number starting from 1970-01-01 UTC we should not do any conversions here. This also means that the test which simply checked thatnew Date(2010, 9, 10)equalsnew DateTime(2010, 10, 10)was wrong. C# does not do any conversions by default, sonew DateTime(2010, 10, 10)does not specificy any timezone. The JavaScript code should match the UTC Date of 2010-10-10 (00:00:00) converted to the local timezone, for example, 2010-10-09 (22:00:00) for MESZ (GMT+2).